fix: replace __uint128_t with portable 64-bit decomposition in c_shard_info.pyx (MSVC build failure) - #950
Conversation
…d_info.pyx
cassandra/c_shard_info.pyx computed the high 64 bits of a 64x32-bit product
by casting biased_token to `__uint128_t` and shifting right by 64:
cdef int shardId = (<__uint128_t>biased_token * self.shards_count) >> 64;
`__uint128_t` is a GCC/Clang compiler-builtin extension type, not standard
C/C++ and not part of Cython's own type system. MSVC has no 128-bit integer
type at all, so it treats `__uint128_t` as an undeclared identifier and fails
with cascading syntax errors (C2065/C2146/C2059) in the generated
c_shard_info.c. This breaks Windows wheel builds for any change that
triggers a rebuild of this extension.
Replaced it with a portable multiply-high decomposition that splits the
64x32-bit multiplication into 32-bit halves, using only 64-bit arithmetic
(uint64_t), matching the existing pure-Python fallback already implemented
in cassandra/shard_info.py. This compiles identically on GCC, Clang, and
MSVC, and is numerically identical to the previous 128-bit computation
(verified against 2M+ random inputs, cross-checked against both the pure
-Python fallback and the actual compiled extension).
Found while investigating an unrelated Windows CI failure on PR scylladb#806; the
bug is pre-existing and independent of that PR's changes, so it's fixed
here as its own standalone commit rather than folded into that PR.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Pull request overview
Replaces a compiler-specific 128-bit multiplication with a portable 64-bit decomposition for shard calculation.
Changes:
- Removes the unsupported
__uint128_tdeclaration. - Computes the product’s high bits using portable 64-bit arithmetic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bug
cassandra/c_shard_info.pyxcomputes the shard id for a token by taking thehigh 64 bits of a 64x32-bit product:
__uint128_tis a GCC/Clang compiler-builtin extension type — it is not partof the C or C++ standard, and it is not a type Cython itself understands
natively (the
cdef extern from *: ctypedef unsigned int __uint128_tblockjust tells Cython to trust that the C compiler has it). MSVC has no 128-bit
integer type at all, so on Windows the generated
c_shard_info.cfails tocompile with
error C2065: '__uint128_t': undeclared identifier, followed bya cascade of C2146/C2059 syntax errors. This breaks Windows wheel builds for
any change that touches (or merely triggers a rebuild of) this extension.
Fix
Replace the 128-bit multiply with a portable multiply-high decomposition that
uses only 64-bit arithmetic (
uint64_t), splitting the multiplication into32-bit halves — the same technique already used by the pure-Python fallback
in
cassandra/shard_info.py(_ShardingInfo.shard_id_from_token). Thiscompiles identically on GCC, Clang, and MSVC.
Verification
cleanly.
shard_id_from_tokenoutputagainst the pure-Python fallback (
cassandra.shard_info._ShardingInfo)across 2,000,000 randomized
(shards_count, sharding_ignore_msb, token)triples, covering the full
int64token range — 0 mismatches.tests/unit/suite (770 passed, 38 skipped, 0 failed) plusthe sharding-specific tests (
tests/unit/test_shard_aware.py,tests/unit/test_connection.py::TestShardawarePortGenerator,tests/unit/test_host_connection_pool.py) — all passing, with the realcompiled extension in place.
Context
This was originally found while investigating a Windows wheel-build CI
failure on an unrelated PR (#806, a
ResponseFuture.__init__cleanup). The__uint128_tbug is pre-existing, independent of that PR's changes, andwould affect the Windows build of any PR that happens to trigger a rebuild
of this extension — so it's being fixed here as its own standalone, focused
PR rather than folded into #806.